Let kernel own default telemetry enabled state - #937
Conversation
Signed-off-by: Jay Xiao <jay.xiao@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Looks sound — the None-passthrough is reachable (client.py never defaults enable_telemetry into kwargs before Session), the is not None guard correctly forwards explicit True/False while omitting unset, and tests are updated to match. One medium concern: this removes a deliberately-documented opt-out default on the kernel path, and the resulting default now depends on an external databricks_sql_kernel value not visible in this repo — worth confirming it matches intent.
| "enable_telemetry": kwargs.get("enable_telemetry", False), | ||
| # Preserve the caller's explicit telemetry choice. When unset, | ||
| # leave it as None so the kernel applies its own default. | ||
| "enable_telemetry": kwargs.get("enable_telemetry"), |
There was a problem hiding this comment.
🟡 Medium — This reverses a previously-deliberate divergence. The old code (and its removed comment) explicitly forced kernel-path telemetry OFF by default — "telemetry_enabled": bool(options.get("enable_telemetry", False)) in client.py — and warned "Do not 'fix' this back to True to match the other backends." With this change, when a use_kernel=True caller does not pass enable_telemetry, the key is omitted and the kernel applies its own default.
The resulting default is not verifiable in this repo — it lives in databricks_sql_kernel. If the kernel defaults telemetry_enabled to True (which would match the connector-wide enable_telemetry default of True at client.py:435), this silently flips kernel telemetry from opt-out to opt-in for users who never set the flag — a privacy-relevant default change that goes beyond the "let kernel own the default" framing.
Please confirm the kernel's built-in default matches the intended behavior here, and consider noting the effective default in the comment (or a test asserting the end-to-end default) so a future kernel-side change to that default can't silently alter connector behavior.
1 participant
Summary
Validation